While I am not opposed to the addition of a polkit integration, I would rather avoid all KDE dependencies as including them leads to massive dependency trees on most distros, and they're usually pretty replaceable.

polkit-qt-1 appears to be a very thin wrapper around the polkit api that doesn't provide much. I would prefer to avoid it unless you have a good reason to pull it in.

polkit-qt-1 is indeed a very thin wrapper. The 'good' reason for using it is that it deals with some of the more error-prone aspects of the upstream libraries and doesn't really bring anything that we don't need. While it is maintained by the KDE team, the package doesn't actually pull in anything but Polkit itself and Qt on the distros I checked (Debian, Ubuntu, Arch, Fedora, RHEL, NixOS, or Gentoo). I find it unlikely that others would have wildly different dependencies.

That said, it should also be fairly easy to not use it at the cost of a few hundred LOC, which would have the benefit of me getting to correct one of the limitations of polkit-qt-1.

It sounds like one of the more reasonable ones if we had to use it then, though I would personally still prefer not to.

Alright. I'll see what I can do.

I removed the dependency on polkit-qt, interacting with the PolKit libraries directly, instead.

Thanks for the review!

I don't like using a gobject library here either, but yes, there is a good reason for using it instead of interacting directly over DBus. PolKit requires the client that sends the confirmation message to be privileged. To this end, libpolkit has a setuid helper binary that it invokes, which handles the actual PAM conversation, and shuffles all of the data between the parent and the helper.

To avoid libpolkit, Quickshell would either need to run as root or we would need our own setuid helper. Besides the implementation overhead, this would break simply running it from your Flake via Nix or installing it via Home-Manager. I imagine (although I don't positively know) many distros might also look more sceptically at any setuid binaries.

Finally, the DBus interface isn't actually stable because it is currently considered an implementation detail. I don't think it has changed significantly in recent years, but I would still prefer the more stable solution.

That's horrible on so many levels.

I... yeah. But that's Polkit for you. There's many things I wish I didn't know about it.

If, given these circumstances, you'd rather not have the code upstream, I'm just going to package it up as a separate library.

I'm fine with the gobject code since its an actual requirement, and this is a feature many people want. I'll finish the review tomorrow probably.

Thank you! I'm not in any rush. Also, fair warning: this is the first 'not insignificant' thing I've done with Qt, so while I tried to do things right, some things might be the way they are because I didn't know any better. I'll happily take any pointers.

Thats fine. One further thing that would be helpful would be to add a manual test qml file similar to what exists in other parts of the source. Just cover a reasonable amount of the api for a simple manual regression test

I got started on some of your comments. Here's an additional thing we might consider:

PolKit can send either 'show-info' or 'show-error'. I am currently storing both into the same property with an isError flag. I.e. an incoming error overwrites a previous info. PolKit itself does not specify the way the UI should look, but many agents handle it this way. However, for some scenarios showing both might make sense, e.g. fprintd will show 'Place your right index finger on the reader' as info and 'Fingerprint did not match' as info. I am considering having both individual properties and a combined one for the latest message so users can choose how to design their dialog.

One thing I'm a bit unsure on is object destruction/cleanup and its behavior in Qt. When an authentication request finishes, we need to clean up all objects, but does this cause the UI to flicker, if we animate a window out, for example?

PolKit can send either 'show-info' or 'show-error'. I am currently storing both into the same property with an isError flag. I.e. an incoming error overwrites a previous info. PolKit itself does not specify the way the UI should look, but many agents handle it this way. However, for some scenarios showing both might make sense, e.g. fprintd will show 'Place your right index finger on the reader' as info and 'Fingerprint did not match' as info. I am considering having both individual properties and a combined one for the latest message so users can choose how to design their dialog.

Does the api provide any sort of hint as to if both should be present or not? If it does something like sending a new version of the error message thats an empty string to reset thats a pretty clear signal we could have multiple, but if not I'm less sure. If the messages are coming from pam, our pam module currently replaces the last message with the new one so it probably makes sense to keep that design.

One thing I'm a bit unsure on is object destruction/cleanup and its behavior in Qt. When an authentication request finishes, we need to clean up all objects, but does this cause the UI to flicker, if we animate a window out, for example?

This is a bit of a mess, Quickshell in particular has a hack for it called Retainable which you can see here https://quickshell.org/docs/v0.2.1/types/Quickshell/Retainable/. Retainable objects generally hold their last state and become immutable while retained to make it easier to write reactive UI against them.

Note: I tested the dialog and see you have a lot of DEBUG: PolkitAgent: lines. Use logging categories (qCDebug etc) for this. We hide them by default but they can be as detailed as you want (though you should make an effort to not put passwords in them)

Does the api provide any sort of hint as to if both should be present or not? If it does something like sending a new version of the error message thats an empty string to reset thats a pretty clear signal we could have multiple, but if not I'm less sure. If the messages are coming from pam, our pam module currently replaces the last message with the new one so it probably makes sense to keep that design.

Unfortunately not. The messages are indeed forwarded from PAM. I am therefore inclined to leave it as is.

This is a bit of a mess, Quickshell in particular has a hack for it called Retainable which you can see here https://quickshell.org/docs/v0.2.1/types/Quickshell/Retainable/. Retainable objects generally hold their last state and become immutable while retained to make it easier to write reactive UI against them.

Thanks! I'll look into that to figure out how best to use it in this scenario.

Note: I tested the dialog and see you have a lot of DEBUG: PolkitAgent: lines. Use logging categories (qCDebug etc) for this. We hide them by default but they can be as detailed as you want (though you should make an effort to not put passwords in them)

That's exactly what I was looking for. I'll migrate to that.

I performed a fairly significant refactoring to accommodate the changes you recommended.

• PolkitAgent is now a very thin wrapper with the business logic living in PolkitAgentImpl.
• PostReloadHook is used to handle transitions between generations.
• State of the currently active request as well as any queued requests is preserved on reload.
• AuthFlow is a new class that houses the actual state of an in-progress authentication. It is Retainable to accommodate animating out dialogs.
• The code is just better organized overall.

I'll look at the lint errors soon.

Thank you for the extensive review! I addressed most of your comments. I also introduced a RAII helper GObjectRef to make handling of GObject objects less error prone. I still need to look into bindable properties.

I selectively turned off some lints in specific instances where I felt they were not helpful or actively wrong (e.g. it won't let me forward declare any structs from the Polkit libraries). There should always be an accompanying comment explaining why the specific rule doesn't make sense in that case. Let me know if you disagree with any of my reasons.

I do love a good reactivity system. I switched over to bindable properties where I figured it makes sense. With that I believe I have addressed everything you pointed out. I appreciate your patience, this has been quite the learning experience.

I rebased onto current master, squashed and fixed a small bug in the reloading logic.

I have also integrated this into Noctalia in a branch to validate in a more complex config.

Minor style question: I have moved the bindable getters to the header, should I do the same for other property getters as well? Other than that I believe I got to everything. I've left it as separate commits for now to make it easier to review, but I can squash later.

Generally my policy is one-line getter functions can go in the header. Takeover change looks fine here.

Alright, thanks! I went through and moved all one-line getters to the headers.

Test issues discovered:

1. Polkit agent does not attempt to register itself if it failed and then the currently active agent leaves the bus. See notifications.

This is true. I didn't tackle that before since it seemed to add significant complexity for relatively little advantage. However, I see the QDbusServiceWatcher actually makes this quite simple. Will do.

2. Debug log levels are still enabled

My bad. I misunderstood how the logging categories worked. I thought the default log level was a runtime switch, not in the code.

3. Supplementary error message is never populated for bad passwords

True. That was an oversight when designing clearState. It never came up for me in testing since my PAM config starts with fingerprints and the show-info from that overwrites any error message on restart anyway. I'll keep the supplementary message in that case. (Side note: I really wish PAM had better structuring to conversations.)

Also noticed: for bindables use Qt::beginPropertyUpdateGroup / end update group to not send useless updates.

Aha! I was looking for something like that, thanks!

For the log levels just add QtWarningMsg to the logging category declaration to set the default. QT_LOGGING_RULES is then usable as a runtime switch.

I have looked into registering once the active agent unregisters. Unfortunately, there is no specified bus names that agents need to register. They simply need to have an object on the system bus implementing the agent interface. Since agents generally deny any introspection request on the system bus, we cannot reliably determine which object or service we need to wait for.

Also, the Polkit authority does not provide any methods for obtaining the active agent, neither over D-Bus nor via other APIs. At best, we could come up with some heuristics based on running processes or simple timeouts. At that point, I would seriously question whether that is reasonable and worth it.